Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change address fields #117 #118

Merged
merged 19 commits into from
Nov 17, 2023
Merged

Change address fields #117 #118

merged 19 commits into from
Nov 17, 2023

Conversation

MatteoGuarnaccia5
Copy link
Contributor

@MatteoGuarnaccia5 MatteoGuarnaccia5 commented Nov 2, 2023 •
edited
Loading

Description

Changed address fields so that it can be used internationally. Updated tests accordingly

Testing instructions

Add a set up instructions describing how the reviewer should test the code

  • Review code
  • Check Actions build
  • Review changes to test coverage
  • {more steps here}

Agile board tracking

to be merged in only when ral-facilities/inventory-management-system-api#85 (comment) merged in
and to be merged in at the same time as ral-facilities/inventory-management-system-api#108
closes #117

@codecov Codecov
Copy link

codecov bot commented Nov 2, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (83b17a6) 91.91% compared to head (c53efea) 91.87%.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop     #118      +/-   ##
===========================================
- Coverage    91.91%   91.87%   -0.04%     
===========================================
  Files           36       36              
  Lines         2077     2055      -22     
  Branches       602      610       +8     
===========================================
- Hits          1909     1888      -21     
+ Misses         166      165       -1     
  Partials         2        2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MatteoGuarnaccia5 MatteoGuarnaccia5 marked this pull request as ready for review November 2, 2023 16:05
@joelvdavies joelvdavies mentioned this pull request Nov 7, 2023
Merged
4 tasks
@joelvdavies joelvdavies mentioned this pull request Nov 8, 2023
Merged
3 tasks
@joelvdavies joelvdavies mentioned this pull request Nov 9, 2023
Merged
3 tasks
Base automatically changed from edit-manufacturers-#73 to develop November 9, 2023 14:27
joelvdavies
joelvdavies requested changes Nov 9, 2023
View reviewed changes
Copy link
Collaborator

@joelvdavies joelvdavies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things I noticed right now, with the types, currently if something is empty in the add dialogue e.g. the town it sends null
image

Ideally this would instead use undefined in typescript so that it isn't sent at all in the request. Its the same sort of thing with the edit requests.

The address display also needs some sort of logic when rendering as null fields appear like
image

src/manufacturer/manufacturerDialog.component.tsx Outdated Show resolved Hide resolved
src/app.types.tsx Show resolved Hide resolved
src/app.types.tsx Outdated Show resolved Hide resolved
src/app.types.tsx Outdated Show resolved Hide resolved
joelvdavies
joelvdavies requested changes Nov 13, 2023
View reviewed changes
Copy link
Collaborator

@joelvdavies joelvdavies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple more type related things. Also could the order of the fields be modified in the dialog/when displaying to have country after county, instead of before the first address line
image
as that is more standard.

src/app.types.tsx Outdated Show resolved Hide resolved
src/app.types.tsx Outdated Show resolved Hide resolved
src/app.types.tsx Outdated Show resolved Hide resolved
src/app.types.tsx Outdated Show resolved Hide resolved
src/manufacturer/manufacturer.component.test.tsx Outdated Show resolved Hide resolved
src/manufacturer/manufacturerDialog.component.tsx Outdated Show resolved Hide resolved
src/manufacturer/manufacturerDialog.component.tsx Outdated Show resolved Hide resolved
src/manufacturer/manufacturerDialog.component.tsx Outdated Show resolved Hide resolved
src/manufacturer/manufacturerDialog.component.tsx Outdated Show resolved Hide resolved
src/manufacturer/manufacturerDialog.component.tsx Outdated Show resolved Hide resolved
joelvdavies
joelvdavies requested changes Nov 14, 2023
View reviewed changes
Copy link
Collaborator

@joelvdavies joelvdavies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I don't think the comment I made was clear on what to remove.

src/app.types.tsx Outdated Show resolved Hide resolved
src/manufacturer/manufacturerDialog.component.tsx Outdated Show resolved Hide resolved
joelvdavies
joelvdavies previously approved these changes Nov 15, 2023
View reviewed changes
joshuadkitenge
joshuadkitenge requested changes Nov 15, 2023
View reviewed changes
Copy link
Collaborator

@joshuadkitenge joshuadkitenge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You currently have two useState for setting the manufacturer details in the manufacturer component . You should only need one useState. (setManufacturer and setSelectedManufacturer)

@joshuadkitenge joshuadkitenge self-requested a review November 15, 2023 17:08
@MatteoGuarnaccia5 MatteoGuarnaccia5 mentioned this pull request Nov 16, 2023
Closed
4 tasks
joelvdavies
joelvdavies reviewed Nov 16, 2023
View reviewed changes
Copy link
Collaborator

@joelvdavies joelvdavies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last note on the types - also speaking to Joshua, we need to merge
ral-facilities/inventory-management-system-api#115 and #128 first as the user feedback is coming up.

Also saw this which is a good sign
image

src/app.types.tsx Outdated Show resolved Hide resolved
src/app.types.tsx Outdated Show resolved Hide resolved
src/app.types.tsx Outdated Show resolved Hide resolved
@joshuadkitenge joshuadkitenge merged commit be26ff7 into develop Nov 17, 2023
3 checks passed
@joshuadkitenge joshuadkitenge deleted the address-field-changed-#117 branch November 17, 2023 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joelvdavies joelvdavies joelvdavies approved these changes

@joshuadkitenge joshuadkitenge joshuadkitenge approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Address input field to become international standard
3 participants
@MatteoGuarnaccia5 @joshuadkitenge @joelvdavies